-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set User-Agent header on both REST and graphql requests #259
Conversation
@jradtilbrook @mcncl absolutely no hurry on this one, I just thought I'd poke at it while the changes in #256 were fresh in our brains. If we merge it, it could wait for the next release you do for other reasons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea @yob!
There are a few minor comments, but I think it's safe to skip these and add https://github.com/golangci/golangci-lint in a separate PR instead.
req.Header[k] = v | ||
} | ||
} | ||
return rt.next.RoundTrip(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RoundTrip
returns an error, so normally we'd want to wrap it to provide additional context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed the other feedback, but left this one. I could wrap the error, but... I really wasn't sure what to add that was meaningful.
Happy to merge once @danstn suggestions are committed in. Also happy to just commit them from comment too. |
They're good suggestions, I'll make them in a bit ❤️ |
In #256 we starting setting a custom User-Agent header on REST API calls, to help us understand the ways our API is used. It looks like this: Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.0.3 buildkite/0.15.0 This builds on that work and sets the same User-Agent on graphql API requests as well. It drops the unnecessary use of golang.org/x/oauth2 to set a static bearer token header, and uses the tripperware pattern [1] to set both headers we care about: 1. the static bearer token for auth 2. the user agent The same http.Client is used for both REST and graphql, so we'll get identical behaviour on both APIs. [1] https://dev.to/stevenacoffman/tripperwares-http-client-middleware-chaining-roundtrippers-3o00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I've confirmed that we see the Terraform useragent in our API access logs for both the REST and graphql calls |
In #256 we starting setting a custom User-Agent header on REST API calls, to help us understand the ways our API is used. It looks like this:
This builds on that work and sets the same User-Agent on graphql API requests as well.
It drops the unnecessary use of golang.org/x/oauth2 to set a static bearer token header, and uses the tripperware pattern to set both headers we care about:
The same http.Client is used for both REST and graphql, so we'll get identical behaviour on both APIs.